feat: Add OpenAPI documentation on controller OAuth2ProviderController web routes#113
feat: Add OpenAPI documentation on controller OAuth2ProviderController web routes#113matiasperrone-exo wants to merge 2 commits intomainfrom
Conversation
5bf7fad to
dfb8863
Compare
martinquiroga-exo
left a comment
There was a problem hiding this comment.
LGTM
@matiasperrone-exo please add clickup card link to this PR
dfb8863 to
1e72293
Compare
📝 WalkthroughWalkthroughAdds OpenAPI PHP attribute annotations to the OAuth2/OIDC controller methods and introduces multiple OpenAPI schema and security classes for OAuth2 requests/responses, JWKS, and OpenID discovery; no runtime controller logic or public method signatures were changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Rebased |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
martinquiroga-exo
left a comment
There was a problem hiding this comment.
@matiasperrone-exo please see comments and please sign your commits or PR will be rejected.
Also, as a final note, PUT, PATCH, and DELETE are documented for /oauth2/auth but are not part of OAuth2/OIDC, so I'm not sure if they are needed but I defer to you.
1e72293 to
db9f777
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Swagger/OAuth2ProviderControllerSchemas.php`:
- Around line 7-172: The response schemas are missing RFC-required "required"
arrays; update the OA\Schema annotations for OAuth2TokenResponseSchema to
include required: ['access_token','token_type'], for
OAuth2IntrospectionResponseSchema add required: ['active'], for
JWKSResponseSchema add required: ['keys'], and for OpenIDDiscoveryResponseSchema
add required:
['issuer','authorization_endpoint','token_endpoint','jwks_uri','response_types_supported','subject_types_supported','id_token_signing_alg_values_supported']
by adding the required key to each corresponding #[OA\Schema(...)] attribute so
the generated docs include the mandated required fields.
In `@app/Swagger/Security/OAuth2ProviderControllerSecuritySchema.php`:
- Line 14: The Swagger OAuth2 security schema is using
L5_SWAGGER_CONST_TOKEN_URL which defaults to http://localhost/oauth/token but
your actual endpoint is /oauth2/token; update the constant value or document the
required env override so the tokenUrl points to /oauth2/token (or the correct
full URL) — locate the usage in OAuth2ProviderControllerSecuritySchema.php
referencing L5_SWAGGER_CONST_TOKEN_URL and change the configuration/ENV (or the
default in the l5-swagger config) so the swagger Authorize flow targets
/oauth2/token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 601a3165-be20-415f-abe8-e4fa29f5c8c5
📒 Files selected for processing (8)
app/Http/Controllers/OAuth2/OAuth2ProviderController.phpapp/Swagger/OAuth2ProviderControllerSchemas.phpapp/Swagger/Requests/OAuth2AuthorizationRequestSchema.phpapp/Swagger/Requests/OAuth2EndSessionRequestSchema.phpapp/Swagger/Requests/OAuth2TokenIntrospectionRequestSchema.phpapp/Swagger/Requests/OAuth2TokenRequestSchema.phpapp/Swagger/Requests/OAuth2TokenRevocationRequestSchema.phpapp/Swagger/Security/OAuth2ProviderControllerSecuritySchema.php
|
@martinquiroga-exo please review again. All PR's requested changes and additional AI comments were added. |
59f315f to
03c7efc
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php`:
- Around line 431-440: The operationId value inside the OA\Get for the
'/oauth2/.well-known/openid-configuration' endpoint is garbled
("OAclient_secretuth2OpenIdDiscovery"); replace it with a proper camelCase
identifier such as "oauth2OpenIdDiscovery" in that OA\Get annotation, and also
change the sibling operationId "OpenIdDiscovery" (earlier in the file) to
"openIdDiscovery" to maintain camelCase consistency with other operationIds like
oauth2Token and oauth2RevokeToken.
- Around line 301-320: The OpenAPI security annotation is incorrect for the
token revoke/introspection/end-session endpoints: OAuth2ProviderSecurity (oauth2
clientCredentials) implies a Bearer token but the implementation
(ClientService::getCurrentClientAuthInfo) accepts HTTP Basic or
client_id/client_secret form params and end-session uses
id_token_hint/client_id; fix by adding a new HTTP Basic security scheme named
OAuth2ProviderClientBasic (type=http, scheme=basic, description per RFC 6749
§2.3.1) and update the annotations on the OAuth2ProviderController operations
for /oauth2/token/revoke and /oauth2/token/introspection to allow either the
OAuth2ProviderClientBasic or existing OAuth2ProviderSecurity (i.e., list both in
the security array), and remove the security annotation entirely from the
/oauth2/end-session operation so that authentication remains documented via
id_token_hint/client_id form/query params.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c11200a1-3918-4c70-8ae4-5eb3bc1ca968
📒 Files selected for processing (4)
app/Http/Controllers/OAuth2/OAuth2ProviderController.phpapp/Swagger/OAuth2ProviderControllerSchemas.phpapp/Swagger/Requests/OAuth2AuthorizationRequestSchema.phpapp/Swagger/Requests/OAuth2TokenIntrospectionRequestSchema.php
✅ Files skipped from review due to trivial changes (1)
- app/Swagger/Requests/OAuth2AuthorizationRequestSchema.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Swagger/OAuth2ProviderControllerSchemas.php
fixed |
03c7efc to
4ce765c
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-113/ This page is automatically updated on each push to this PR. |
|
@martinquiroga-exo please review again |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/Http/Controllers/OAuth2/OAuth2ProviderController.php (1)
426-426:⚠️ Potential issue | 🟡 MinorStale "Also available at …" sentence still present in description.
A previous review asked to remove
Also available at /oauth2/.well-known/openid-configuration.from this description to avoid inconsistency, and the reply was "done" — but the sentence is still here.✏️ Proposed fix
- description: 'Returns the OpenID Provider Configuration document per OpenID Connect Discovery 1.0. Also available at /oauth2/.well-known/openid-configuration.', + description: 'Returns the OpenID Provider Configuration document per OpenID Connect Discovery 1.0.',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php` at line 426, The description string inside OAuth2ProviderController (the OpenID Provider Configuration response) still contains the stale sentence "Also available at /oauth2/.well-known/openid-configuration."; locate the description assignment or array entry (the string shown in the diff) in OAuth2ProviderController.php and remove that trailing sentence so the description reads only "Returns the OpenID Provider Configuration document per OpenID Connect Discovery 1.0." (ensure punctuation remains correct).
🧹 Nitpick comments (1)
app/Http/Controllers/OAuth2/OAuth2ProviderController.php (1)
258-258: Consider allowingOAuth2ProviderClientBasicon/oauth2/tokenfor consistency.The sibling
/oauth2/token/revoke(line 308) and/oauth2/token/introspection(line 358) endpoints list bothOAuth2ProviderClientBasicandOAuth2ProviderSecurity, but/oauth2/tokenlists onlyOAuth2ProviderSecurity. The token endpoint accepts HTTP Basic client authentication per RFC 6749 §2.3.1 as well, so Swagger UI should surface that option here too.🔧 Suggested alignment
- security: [['OAuth2ProviderSecurity' => []]], + security: [['OAuth2ProviderClientBasic' => []], ['OAuth2ProviderSecurity' => []]],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php` at line 258, The OpenAPI security entry for the /oauth2/token operation currently lists only 'OAuth2ProviderSecurity'; update the operation's security array to also include 'OAuth2ProviderClientBasic' so the token endpoint surfaces HTTP Basic client auth like the sibling endpoints (/oauth2/token/revoke and /oauth2/token/introspection). Locate the security declaration for the /oauth2/token operation in OAuth2ProviderController (the entry with security: [['OAuth2ProviderSecurity' => []]]) and add an additional element for 'OAuth2ProviderClientBasic' to the array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php`:
- Line 426: The description string inside OAuth2ProviderController (the OpenID
Provider Configuration response) still contains the stale sentence "Also
available at /oauth2/.well-known/openid-configuration."; locate the description
assignment or array entry (the string shown in the diff) in
OAuth2ProviderController.php and remove that trailing sentence so the
description reads only "Returns the OpenID Provider Configuration document per
OpenID Connect Discovery 1.0." (ensure punctuation remains correct).
---
Nitpick comments:
In `@app/Http/Controllers/OAuth2/OAuth2ProviderController.php`:
- Line 258: The OpenAPI security entry for the /oauth2/token operation currently
lists only 'OAuth2ProviderSecurity'; update the operation's security array to
also include 'OAuth2ProviderClientBasic' so the token endpoint surfaces HTTP
Basic client auth like the sibling endpoints (/oauth2/token/revoke and
/oauth2/token/introspection). Locate the security declaration for the
/oauth2/token operation in OAuth2ProviderController (the entry with security:
[['OAuth2ProviderSecurity' => []]]) and add an additional element for
'OAuth2ProviderClientBasic' to the array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9443d97-061c-43d3-8ad1-f82dd57c2e97
📒 Files selected for processing (2)
app/Http/Controllers/OAuth2/OAuth2ProviderController.phpapp/Swagger/Security/OAuth2ProviderControllerSecuritySchema.php
Task:
Ref: https://app.clickup.com/t/86b7fjj9a
Summary by CodeRabbit